Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests/e2e: allow to superseed images in operator.sh #341

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wainersm
Copy link
Member

I started working on adapting the Kata CI scripts to install from this operator. I will be trying to re-use the scripts in tests/e2e/ as much as possible, for example, tests/e2e/operator.sh install to install and check Kata Containers in the cluster's nodes. So far I faced two problems with operator.sh though:

  1. one cannot overwrite the default kata-payload image. And it will be needed to point the operator to the image built by the kata CI, if it runs in a pull-request triggered job;
  2. likewise one cannot overwirte the operator controller and reqs-payload images. For those images there is an extra problem which is operator.sh uses the built-and-pushed-to-local-registry images so it always tries to start a local registry service. If it is installing from quay.io (or ghcr.io), for example, it makes no sense the local registry.

So this PR address those two problems. Also by making operator.sh more flexible, this will likely to solve issues when we start improving the operator CI pipeline as suggested in #309

If that helps on review, here goes a sample of the code I'm writing for the Kata CI side:

function install_coco_operator() {
	local repo

	repo=$(get_from_kata_deps "externals.coco-operator.url")
	if [ -f "$COCO_OPERATOR_DIR" ]; then
		rm -rf "$COCO_OPERATOR_DIR"
	fi
	git clone "$repo" "$COCO_OPERATOR_DIR"
	pushd "$COCO_OPERATOR_DIR"
		# Let's install the kustomize tool if not present.
		make kustomize
		echo "::group::Install operator"
		PATH="$PATH:${PWD}/bin" echo "./tests/e2e/operator.sh install"
		echo "::endgroup::"
	popd
}

I tested locally by running:

$ IMG="quay.io/confidential-containers/operator:latest" \
PRE_INSTALL_IMG="quay.io/confidential-containers/reqs-payload:latest" \
KATA_PAYLOAD_IMG="quay.io/kata-containers/kata-deploy-ci:kata-containers-latest" \
PATH=$PATH:${PWD}/bin \
./tests/e2e/operator.sh install

`./tests/e2e/operator.sh install` is still tightly coupled with the
build phase, which happen to push images to a local registry, so the
install tries to always start the registry service. Now suppose that you
want to run `./tests/e2e/operator.sh install` stand-alone to install
images from quay.io, then the registry service locally is not needed.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
By exporting $KATA_PAYLOAD_IMG variable the `./tests/e2e/operator.sh
install` will replace the default kata-payload image.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks Wainer

@portersrc
Copy link
Member

lgtm

@@ -303,6 +313,9 @@ usage() {
"install": install only,
"wait_for_stabilization": wait for CoCo pods to be stable
"uninstall": uninstall the operator.
environment variables :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this section, could you please (in a separate commit) add the RUNTIMECLASS, which is already supported to be set?

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks good, only the already-supported-to-be-set variable is missing in the usage message. Could you please add it (Ideally as a first commit to introduce this section, alternatively just as a part of the current first commit which defines the section)

@GabyCT
Copy link
Contributor

GabyCT commented Jan 29, 2024

lgtm

Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except from the @ldoktor comments, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants